Austenem/CAT-246 dataset detail workspace updates #3533
Conversation
There was a problem hiding this comment.
A few comments for now. I'll definitely need @NickAkhmetov to take a look at this as he's much closer to the unified datasets work.
context/app/static/js/components/detailPage/ProcessedData/ProcessedDataWorkspaceMenu.tsx
Outdated
Show resolved
Hide resolved
context/app/static/js/components/detailPage/ProcessedData/ProcessedDataWorkspaceMenu.tsx
Outdated
Show resolved
Hide resolved
| { | ||
| children: 'Add to Workspace', | ||
| onClick: trackAddToWorkspace, | ||
| icon: <AddRounded color="primary" />, |
There was a problem hiding this comment.
We don't need the fontSize prop here?
There was a problem hiding this comment.
Adding a fontSize prop of any value to this icon shrinks it, not sure why - but the default size matches designs.
There was a problem hiding this comment.
Icons directly imported from @mui/material/icons have a default fontSize of 1.25rem (medium): https://mui.com/material-ui/icons/#size
Icons we import from our icon map have their default font size set to 1rem by the default icon styles, so we end up needing to add the fontSize prop for those cases.
...s/components/detailPage/entityHeader/EntityHeaderActionButtons/EntityHeaderActionButtons.tsx
Outdated
Show resolved
Hide resolved
| // Clone the button element and add the onClick handler | ||
| const buttonWithClickHandler = React.cloneElement(button as React.ReactElement, { | ||
| onClick: handleClick, | ||
| 'aria-controls': open ? 'basic-menu' : undefined, | ||
| 'aria-haspopup': 'true', | ||
| 'aria-expanded': open ? 'true' : undefined, | ||
| }); |
There was a problem hiding this comment.
Why can't we just pass these as props to a mui Button component?
There was a problem hiding this comment.
Because the cases for this component involve very differently styled menu buttons, I opted to let that button be a fully customizable React node prop (screen caps below). This cloned element was an effort to add as many shared properties to that customized button as possible and avoid duplication.
context/app/static/js/components/detailPage/ProcessedData/ProcessedDataWorkspaceMenu.tsx
Outdated
Show resolved
Hide resolved
| defaultName: hubmap_id, | ||
| initialSelectedDatasets: [uuid] ?? [], |
There was a problem hiding this comment.
The uuid and hubmap_id will always be the primary dataset since it comes from the flask data context. Doesn't the helper panel only appear for the current processed dataset in view?
There was a problem hiding this comment.
Yes - there is a zustand store that provides the current visible helper panel content.
Also, [uuid] is never nullish due to list wrapper; if we want to fall back to an empty array in this case, [uuid].filter(Boolean) should work.
There was a problem hiding this comment.
Got it- updated this to reflect the current processed dataset.
context/app/static/js/components/detailPage/ProcessedData/ProcessedDataWorkspaceMenu.tsx
Show resolved
Hide resolved
context/app/static/js/components/detailPage/ProcessedData/ProcessedDataWorkspaceMenu.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good to me. Let's wait on @NickAkhmetov's review before merging.
context/app/static/js/components/detailPage/ProcessedData/ProcessedDataWorkspaceMenu.tsx
Show resolved
Hide resolved
austenem
deleted the
austenem/cat-246-dataset-detail-workspace-updates
branch
|
Thanks! |
Summary
Update menu choices for the workspace buttons in unified views dataset detail pages to allow for both creating and editing workspaces.
Design Documentation/Original Tickets
CAT-246 Jira ticket
CAT-507 Jira ticket (design)
Testing
Went through both options for both workspace buttons on several dataset pages.
Screenshots/Video
Screen.Recording.2024-09-05.at.12.29.05.PM.mov
Checklist
CHANGELOG-your-feature-name-here.mdis present in the root directory, describing the change(s) in full sentences.